-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Deprecate withers in 2.x in favor of setters #3756
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jhl221123,
Thanks for the PR! This is a great step toward cleaning up the inconsistent use of set
and with
methods across the codebase.
A few comments and suggestions:
-
In Fix(#2769): Modify the annotation processor in 2.x to give a warning if a plugin builder attribute does not have a public setter. #3195, we updated the
PluginProcessor
to throw a compilation error if a configuration field doesn't have a publicset
orwith
method. Could you update that check to only allowset
methods going forward? -
Making that change will also help catch any remaining
with
methods that may have been missed. For example,AsyncWaitStrategyConfig
andHtmlLayout
still contain withers that should be removed. -
Minor: We follow a consistent documentation approach when deprecating methods:
- Add
@deprecated since <version>
in the Javadoc of the deprecated method. - Add
@since <version>
to the new method replacing it.
I should have mentioned that in the issue description, sorry for the additional work this causes.
- Add
-
Minor: I noticed several methods include
@SuppressWarnings("hiding")
on parameters. This likely stems from a differing naming strategy among some contributors. Since we always use the same name for method parameters and their corresponding fields, these annotations aren't needed and just add noise. Could you remove them?
Thanks again for your work on this!
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/AbstractFileAppender.java
Show resolved
Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/AbstractFileAppender.java
Outdated
Show resolved
Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/AppenderSet.java
Outdated
Show resolved
Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/AppenderSet.java
Outdated
Show resolved
Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/AppenderSet.java
Outdated
Show resolved
Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/AppenderSet.java
Outdated
Show resolved
Hide resolved
Hi @ppkarwasz, Thank you for the detailed feedback! I've pushed the updates with all your suggestions. This was a great chance to get a better understanding of the project.
The current tone is perfect. Thank you for being so considerate! |
Busted 😉! As you probably guessed I use the assistance of AI to format my suggestions faster. The message might be AI-assisted, but the appreciation is real. Keep the good work! 💯 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The BND Baseline plugins shows that you also need to bump the version of the org.apache.logging.log4j.core.async
package to 2.26.0
. You didn't modify it directly, but it gained additional methods through class inheritance.
Otherwise, this looks ready to be merged, thanks!
This pull request addresses issue #3750 by deprecating all withers in builder classes and introducing setters as replacements. All internal usages and related tests have been updated to use the setters.
Important
Base your changes on
2.x
branch if you are targeting Log4j 2; usemain
otherwise.Checklist
Before we can review and merge your changes, please go through the checklist below. If you're still working on some items, feel free to submit your pull request as a draft—our CI will help guide you through the remaining steps.
✅ Required checks
License: I confirm that my changes are submitted under the Apache License, Version 2.0.
Commit signatures: All commits are signed and verifiable. (See GitHub Docs on Commit Signature Verification).
Code formatting: The code is formatted according to the project’s style guide.
How to check and fix formatting
./mvnw spotless:check
./mvnw spotless:apply
See the build instructions for details.
Build & Test: I verified that the project builds and all unit tests pass.
How to build the project
Run:
./mvnw verify
See the build instructions for details.
🧪 Tests (select one)
📝 Changelog (select one)
src/changelog/.2.x.x
. (See Changelog Entry File Guide).